Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update mypy #153

Merged
merged 1 commit into from
Apr 26, 2019
Merged

Update mypy #153

merged 1 commit into from
Apr 26, 2019

Conversation

njgheorghita
Copy link
Contributor

What was wrong?

New mypy version has been released, was updated along with other linting dependencies

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the mypy branch 3 times, most recently from 959cf8c to 781a8c0 Compare April 25, 2019 11:19
all_link_refs = self.linked_references
else:
all_link_refs = self.unlinked_references
unlinked_refs = self.unlinked_references if self.unlinked_references else ({},)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be shortened to `unliked_refs = self.unlinked_references or ({},)

@@ -15,11 +15,11 @@ class LinkableContract(Contract):
contract factories with link references in their package's manifest.
"""

unlinked_references: Tuple[Dict[str, Any]] = None
linked_references: Tuple[Dict[str, Any]] = None
unlinked_references: Optional[Tuple[Dict[str, Any]]] = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the inner dict be mutable? If not maybe convert to FrozenDict at which point you could assign (frozendict(),) as a class property since it's immutable and then not have to worry about the setting of the default value in the constructor.

@@ -32,7 +32,8 @@ def __contains__(self, key: str) -> bool:

def get(self, key: str) -> Dict[str, str]:
self._validate_name_and_references(key)
return self.deployment_data.get(key)
# type ignored b/c _validate_name_and_references enforces key exists
return self.deployment_data.get(key) # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the comment above why not use return self.deployment_data[key] since we know the key exists?

@@ -130,7 +132,7 @@ def manifest_version(self) -> str:
return self.manifest["manifest_version"]

@property
def uri(self) -> str:
def uri(self) -> Optional[str]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Optional? Assuming this is to take into account packages initialized without a URI, what if we just generated a data URI for those? Data URIs could always be valid since they contain their content.

@@ -112,7 +112,7 @@ def validate_manifest_exists(manifest_id: str) -> None:
)


def format_manifest(manifest: Manifest, *, prettify: bool) -> str:
def format_manifest(manifest: Manifest, *, prettify: Optional[bool]) -> str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to do prettify: bool = None which mypy will interpret as Optional. Otherwise the function signature doesn't really make sense since at the python level you still are required to provide the argument but it is allowed to be None (which is weird)

@@ -180,5 +180,6 @@ def get_ipfs_backend(import_path: str = None) -> BaseIPFSBackend:

def get_ipfs_backend_class(import_path: str = None) -> Type[BaseIPFSBackend]:
if import_path is None:
import_path = os.environ.get("ETHPM_IPFS_BACKEND_CLASS", DEFAULT_IPFS_BACKEND)
backend_cls = os.environ.get("ETHPM_IPFS_BACKEND_CLASS", DEFAULT_IPFS_BACKEND)
return import_string(backend_cls)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this needed?

@njgheorghita njgheorghita merged commit 81ed58d into ethpm:master Apr 26, 2019
@njgheorghita njgheorghita deleted the mypy branch April 26, 2019 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants